[PM-12436] replace node keytar - #1012
Conversation
…rate from keytar. Update .gitignore for Rust artifacts, adjust package.json for new build scripts, and modify workflows for native module compilation. Enhance state versioning to support migration of credentials from keytar to desktop_core.
…Script configurations and package dependencies. Refactor native binding logic and enhance error handling. Remove legacy keytar references and adjust package-lock.json for new native module structure.
…removing unnecessary semicolons. Update build scripts in package.json to exclude JavaScript during native module compilation.
…ule directly in the renderer process, removing the need for the credential storage listener. Update TypeScript definitions and enhance error handling in password management functions. Adjust Cargo dependencies and versions for improved compatibility.
|
Claude finished @BTreston's task in 6m 1s —— View job Code Review: PM-12436 replace node keytar
Overall Assessment: REQUEST CHANGES (due to existing unresolved findings) This PR replaces the deprecated No new findings beyond those already flagged in prior reviews. Three existing unresolved threads remain and should be addressed before merge. Active Issues (3 unresolved threads)
Previously Flagged — Now Resolved (13 threads)
|
|
Great job! No new security vulnerabilities introduced in this pull request |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… migration logic - Upgraded `@yao-pkg/pkg` to version `^6.14.2` and updated related dependencies in `package-lock.json`. - Modified CLI packaging scripts in `package.json` to target Node.js 22 for Windows, macOS, and Linux. - Enhanced state migration service to improve handling of secure storage keys, ensuring compatibility with previous versions. - Adjusted GitHub Actions workflow to package the CLI for macOS ARM64 architecture.
- Removed the `ElectronRendererSecureStorageService` and replaced it with `NativeSecureStorageService` for improved secure storage handling. - Updated the service module to use the new storage service, ensuring compatibility with the application name constant. - This change simplifies the secure storage logic and enhances maintainability.
- Added a try-catch block to handle JSON parsing errors when retrieving stored values. - Improved logging to provide clearer error messages when parsing fails, returning null in such cases.
- Added coverage-ldap directory to ESLint ignore list. - Enhanced error logging in NativeSecureStorageService for better clarity on JSON parsing failures. - Cleaned up whitespace in stateMigration.service.ts and services.module.ts for improved readability.
- Replaced manual file watching with electron-reload for improved performance and simplicity. - Removed unnecessary fs.watch implementation, streamlining the codebase.
- Updated CLI packaging scripts in `package.json` to target platform-specific identifiers for Windows, macOS, and Linux. - Removed the macOS ARM64 build step from the GitHub Actions workflow to streamline the CI process. - Adjusted the version of `@yao-pkg/pkg` in `package.json` for compatibility. - Cleaned up comments in `state.model.ts` for better clarity.
- Simplified the migration process from keytar's UTF-8 format to desktop_core's UTF-16 format. - Introduced a new helper function `get_password_keytar` to handle credential retrieval. - Improved error handling and streamlined the migration function to enhance readability and maintainability.
| let password = unsafe { | ||
| std::str::from_utf8_unchecked(std::slice::from_raw_parts( | ||
| (*credential).CredentialBlob, | ||
| (*credential).CredentialBlobSize as usize, | ||
| )) | ||
| }; |
There was a problem hiding this comment.
from_utf8_unchecked is undefined behavior in Rust if the credential blob contains any non-UTF-8 bytes.
Details and fix
If the Windows Credential Manager entry is corrupted or was written by something other than keytar, the blob may not be valid UTF-8. Using from_utf8_unchecked in that case is UB per the Rust reference, potentially causing memory safety violations.
Replace with the checked variant:
| let password = unsafe { | |
| std::str::from_utf8_unchecked(std::slice::from_raw_parts( | |
| (*credential).CredentialBlob, | |
| (*credential).CredentialBlobSize as usize, | |
| )) | |
| }; | |
| let password = unsafe { | |
| std::str::from_utf8(std::slice::from_raw_parts( | |
| (*credential).CredentialBlob, | |
| (*credential).CredentialBlobSize as usize, | |
| )) | |
| .map_err(|e| anyhow::anyhow!("Credential blob is not valid UTF-8: {}", e))? | |
| }; |
This returns an error instead of UB when the blob is not valid UTF-8, allowing migrate_keytar_password to gracefully skip that credential (since it catches Err).
- Downgraded `@yao-pkg/pkg` to version `6.5.1` for compatibility. - Updated related dependencies in `package-lock.json` to ensure stability. - Simplified the state migration logic by removing unnecessary keytar password migration code, while retaining essential key mappings for secure storage.
- Moved the import of `passwords` from "dc-native" to a more appropriate location within the file for better organization and readability.
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Great work! Just 2 things to action below (do not do the rust change in this PR).
Also:
- lint is failing
- some builds are failing, they look like they still depend on keytar. e.g. https://github.com/bitwarden/directory-connector/actions/runs/24210945042/job/70679266955?pr=1012
|
|
||
| pub async fn migrate_keytar_password(service: &str, account: &str) -> Result<bool> { | ||
| let value = match get_password_keytar(service, account) { | ||
| Err(_) => return Ok(false), |
There was a problem hiding this comment.
It would be better to return the error, but let's do that in a smaller follow-up PR so as not to block this one.
- Removed unnecessary storage keys from the state migration service to streamline the migration process. - Updated the main.ts file to disable ESLint for the electron-reload import, improving code clarity while maintaining functionality.
…ved code clarity.
| - name: Build native module | ||
| run: npm run build:native |
There was a problem hiding this comment.
Details and fix
The build.yml workflow installs both the Rust toolchain (dtolnay/rust-toolchain@stable) and Linux system dependencies (libdbus-1-dev, libsecret-1-dev, pkg-config) before building the native module. This workflow does neither.
napi build invokes cargo build, which requires the Rust compiler and linker access to libsecret-1-dev and libdbus-1-dev headers. While the GitHub-hosted runner may have a Rust toolchain cached, it does not include the -dev header packages needed for compilation.
Add the missing setup steps before the native build:
- name: Set up Rust
uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8 # stable
- name: Set up system dependencies
run: |
sudo apt-get update
sudo apt-get -y install libdbus-1-dev libsecret-1-dev pkg-config
- name: Build native module
run: npm run build:native- Updated the Node.js setup action to remove the installation of node-gyp, simplifying the process. - Enhanced build triggers in `build-pr.yml` and `build-target.yml` to include additional paths for better coverage during CI.
- Changed the parameter for CRED_FLAGS from a direct value to an Option type for improved type safety and clarity in the credential retrieval process.
|
|
As a note, the builds will fail via the GitHub checks because it pulls in the |
9 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12436
📔 Objective
Rid of node keytar from Directory Connector